cmake: do not build tests for Release build and cleanups #5916
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Appveyor seems to be broken by the PR. |
|
@riversand963 could you help review this change? |
|
ping? |
|
@riversand963 @siying , is there anything i can do to progress this change? i just found that #6098 was merged. it addresses this issue in a different way. but i think this this change is still relevant. |
tchaikov
force-pushed
the
wip-cmake
branch
2 times, most recently
from
e4a8d28
to
27a7a44
Compare
|
Will take a look. |
siying
reviewed
Dec 10, 2019
|
|
||
| add_executable(db_bench${ARTIFACT_SUFFIX} | ||
| tools/db_bench.cc | ||
| tools/db_bench_tool.cc |
There was a problem hiding this comment.
I'm a little bit confused why it can still build after removing this file.
There was a problem hiding this comment.
my bad, added db_bench_tool.cc back . the reason it built is that gflags is not found by cmake as neither do we have Findgflags.cmake nor {gflagsConfig,gflags-config}.cmake in the (ancient) xenial CI node created by travis.
i added Findgflags.cmake to address this issue in this PR.
tchaikov
force-pushed
the
wip-cmake
branch
2 times, most recently
from
e6dd0ad
to
7273261
Compare
|
Travis fails. Can you take a look? |
tchaikov
force-pushed
the
wip-cmake
branch
2 times, most recently
from
6194007
to
a7a1d05
Compare
|
@siying yes. fixed, rebased and repushed. |
cmake pass '-DNDEBUG' to compiler when compiling non-Debug builds. and rocksdb's tests are short-circuited if the "NDEBUG" macro is defined, so the "Release" builds fail when building tests. in this change, cmake disables `WITH_TESTS` option if `CMAKE_BUILD_TYPE` is `Release`. this also maps how `Makefile` defines `dbg` and `release` targets. in `Makefile`, `release` target does not depend on `$(TESTS)`, while `dbg` does. fixes facebook#2445 Signed-off-by: Kefu Chai <tchaikov@gmail.com>
so we can reference it without worrying about its dependencies like gtest. testharness is used by table_reader_bench. Signed-off-by: Kefu Chai <tchaikov@gmail.com>
`WITH_GFLAGS` is enabled/disabled depending on the building system, so it'd be simpler if we could define it using `CMAKE_DEPENDENT_OPTION`. Signed-off-by: Kefu Chai <tchaikov@gmail.com>
unfold the loop, and remove the unused dependencies from linked libraries. Signed-off-by: Kefu Chai <tchaikov@gmail.com>
TEST_EXES and C_TEST_EXES are just aliases, so remove them Signed-off-by: Kefu Chai <tchaikov@gmail.com>
* no need to use a loop for building it * no need to set properties like `EXCLUDE_FROM_DEFAULT_BUILD_RELEASE`, these properties are not read by anybody * no need to link against testutillib Signed-off-by: Kefu Chai <tchaikov@gmail.com>
Signed-off-by: Kefu Chai <tchaikov@gmail.com>
newer libgflags-dev ships with cmake support, but libgflags-dev 2.1.2-3 shipped by xenial does not. let's add the find_package() support. so our CI can build with gflags Signed-off-by: Kefu Chai <tchaikov@gmail.com>
Signed-off-by: Kefu Chai <tchaikov@gmail.com>
as we don't have libgflags-dev installed on mingw, and the toolchain is not MSVC. please note, currently WITH_GFLAGS is OFF by default on MINGW, but it's still better to be explicit. Signed-off-by: Kefu Chai <tchaikov@gmail.com>
since WITH_GFLAGS is ON by default on OSX, we need to install or or disable it explicitly. it'd be better to install it for better coverage of the CI test. Signed-off-by: Kefu Chai <tchaikov@gmail.com>
stress_tool/batched_ops_stress.cc:178:41: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
2822 std::array<std::string, 10> keys = {"0", "1", "2", "3", "4",
2823 ^~~~~~~~~~~~~~~~~~~~~~~~
28241 error generated.
see the discussions at
https://stackoverflow.com/questions/22501368/why-wasnt-a-double-curly-braces-syntax-preferred-for-constructors-taking-a-std
and
https://en.cppreference.com/w/cpp/container/array
Signed-off-by: Kefu Chai <tchaikov@gmail.com>
There was a problem hiding this comment.
@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Thank you for your contribution and your patient! |
|
This pull request has been merged in ac304ad. |
wolfkdy
pushed a commit
to wolfkdy/rocksdb
that referenced
this pull request
Dec 22, 2019
Summary: fixes facebook#2445 Pull Request resolved: facebook#5916 Differential Revision: D19031236 fbshipit-source-id: bc3107b6b25a01958677d7cb411b1f381aae91c6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #2445